London | 26-ITP-Jan | Abdul Moiz | Sprint 2 | Book Library by A-Moiz · Pull Request #385 · CodeYourFuture/Module-Data-Flows

A-Moiz · 2026-03-03T14:26:26Z

Learners, PR Template

Self checklist

I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
My changes meet the requirements of the task
I have tested my changes
My changes follow the style guide

Changelist

Resolved all known bugs
Reset all values after adding book to improve UX

Questions

N/A

ykamal

Nice work! 👍 Things look mostly right. Well done. There are a few things that need a bit of work. I've left comments below. Let me know if you have any questions.

ykamal · 2026-03-16T22:22:04Z

 function populateStorage() {
-  if (myLibrary.length == 0) {
+  if (myLibrary.length === 0) {
    let book1 = new Book("Robison Crusoe", "Daniel Defoe", "252", true);


Love to see some OOP code. Since the class you're referencing here is declared further down the file, for the sake of readability and code organisation, I recommend moving the class definition to much higher, or to the beginning of this file.

ykamal · 2026-03-16T22:23:35Z

 function populateStorage() {
-  if (myLibrary.length == 0) {
+  if (myLibrary.length === 0) {
    let book1 = new Book("Robison Crusoe", "Daniel Defoe", "252", true);


book1 is not getting reassigned later, so using a const keyword will make it safer in the same scope. It's still going to allow you to run object mutations where applicable. but it ensures that you cannot redeclare a variable with the same name inside the same scope.
Same for book2

ykamal · 2026-03-16T22:27:46Z

-    pages.value == null ||
-    pages.value == ""
-  ) {
+  if (title.value === "" || author.value === "" || pages.value === "") {


Good validation check 👍
However, if you do not remove unnecessary whitespaces from the start and the end, " " will pass through this check as it's a bunch of valid characters and is not an empty string. How do you think you could counter this?

I don't think this has been adressed

ykamal · 2026-03-16T22:29:31Z

+    let book = new Book(
+      title.value,
+      author.value,
+      String(pages.value),


This is good safe practice, but there is most likely no need to convert this into a string, as all input values are always returned (usually) as strings, unless something like this is used: https://developer.mozilla.org/en-US/docs/Web/API/HTMLInputElement/valueAsNumber

ykamal · 2026-03-16T22:30:30Z

+    this.title = title;
+    this.author = author;
+    this.pages = pages;
+    this.check = check;


Since this is JavaScript, do you think we should ensure that these constructor arguments are provided in the right data type?

and data range (numbers)

ykamal · 2026-03-16T22:33:49Z

render() appears to be called twice on page load. Perhaps you should create a sequence of this from the root function call and not inside here?

ykamal · 2026-03-16T22:40:33Z

+    } else {
+      readStatus = "Yes";
    }
    changeBut.innerText = readStatus;


Have you come across Ternary Operator? Could make the code cleaner:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Conditional_operator

@A-Moiz there's an easier way to do this:

const myBool = anotherBool ? "Yes" : "No";

ykamal · 2026-03-16T22:45:13Z

-    delBut.className = "btn btn-warning";
-    delBut.innerHTML = "Delete";
-    delBut.addEventListener("clicks", function () {
+    delButton.id = i + 5;


I can see that you are trying to avoid duplicate IDs. Nice approach. However, the approach is hard-coding a specific number, and the outcome could be inconsistent.
You could just use delButton.id = "delButton_" + i or such tactics. That'll make them truly unique. Obviously, use string interpolation with backticks. I can't write that here.

ykamal · 2026-03-16T22:47:57Z

+    delButton.id = i + 5;
+    deleteCell.appendChild(delButton);
+    delButton.className = "btn btn-warning";
+    delButton.innerHTML = "Delete";


use textContent as it parses the provided value as a string, whereas innerHTML parses it as HTML and should only be used if the change contains actual HTML. You can also use innerText for static text. textContent may be faster for most things.

ykamal · 2026-03-16T22:49:36Z

+    deleteCell.appendChild(delButton);
+    delButton.className = "btn btn-warning";
+    delButton.innerHTML = "Delete";
+    delButton.addEventListener("click", function (e) {


a better way to do this would be using data- attributes. You're using i inside a closure which, while fine, is not the best practice.
jQuery has excellent APIs for using data attributes. Even with core JavaScript, if you can master data attributes, you will no longer need to do these, and can use HTML itself for storing that reference.
https://developer.mozilla.org/en-US/docs/Web/HTML/How_to/Use_data_attributes

This is much more appropriate as this isn't dealing with stale data inside a closure

A-Moiz · 2026-03-18T00:19:22Z

Hi @ykamal , I've made all the changes you requested. Let me know if you anymore recommendations.

ykamal · 2026-04-03T09:02:52Z

Apologies, I have not been well. Going to look now.

ykamal · 2026-04-03T09:06:54Z

+    } else {
+      readStatus = "Yes";
    }
    changeBut.innerText = readStatus;


@A-Moiz there's an easier way to do this:

const myBool = anotherBool ? "Yes" : "No";

ykamal · 2026-04-03T09:07:05Z

-    pages.value == null ||
-    pages.value == ""
-  ) {
+  if (title.value === "" || author.value === "" || pages.value === "") {


I don't think this has been adressed

ykamal · 2026-04-03T09:07:40Z

+    deleteCell.appendChild(delButton);
+    delButton.className = "btn btn-warning";
+    delButton.innerHTML = "Delete";
+    delButton.addEventListener("click", function (e) {


This is much more appropriate as this isn't dealing with stale data inside a closure

A-Moiz · 2026-04-06T23:49:36Z

Hi @ykamal , thanks for your comments! I've made the changes. Please have another look and let me know what you think.

cjyuan · 2026-04-18T15:17:41Z

@A-Moiz It seems your changes have addressed @ykamal comments, I will mark this PR as "Complete" on behalf of the reviewer.

illicitonion · 2026-05-04T16:59:18Z

Closing PR because the January ITP run has finished. Feel free to re-open if you're still working on it.

Resolved all known bugs

9e9749a

A-Moiz added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 3, 2026

ykamal added Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Mar 16, 2026

ykamal suggested changes Mar 16, 2026

View reviewed changes

ykamal added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Mar 16, 2026

Made changes based on reviewers comments

36bcbcb

A-Moiz added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Mar 18, 2026

A-Moiz mentioned this pull request Mar 18, 2026

London | Data Flows | Abdul Moiz A-Moiz/My-Coursework-Planner#97

Open

ykamal suggested changes Apr 3, 2026

View reviewed changes

ykamal added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Apr 3, 2026

Responded to comments on GitHub

3fc74b4

A-Moiz added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Apr 6, 2026

cjyuan added Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Apr 18, 2026

illicitonion closed this May 4, 2026

Uh oh!

Conversation

A-Moiz commented Mar 3, 2026

Learners, PR Template

Changelist

Questions

Uh oh!

ykamal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

A-Moiz commented Mar 18, 2026

Uh oh!

ykamal commented Apr 3, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

A-Moiz commented Apr 6, 2026

Uh oh!

cjyuan commented Apr 18, 2026

Uh oh!

illicitonion commented May 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants